-
Notifications
You must be signed in to change notification settings - Fork 21
Initial change handle functionality #1434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| gap: 6px; | ||
| } | ||
|
|
||
| .dialogLoadingSpinnerContainer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maintainability]
Using position: absolute; for .dialogLoadingSpinnerContainer without specifying a top, right, or bottom value other than bottom: 0; can lead to layout issues if the parent container's size changes. Consider ensuring the parent container has a defined size or constraints to prevent unexpected behavior.
| props.setOpen(false) | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isLoading]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The handleClose function is missing props.setOpen in its dependency array. This could lead to stale closures if setOpen changes. Consider adding props.setOpen to the dependency array.
| const onSubmit = useCallback(() => { | ||
| setShowConfirm(true) | ||
| }, []) | ||
| const currentHandle = props.userInfo.handle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
The newHandle value is derived from getValues() outside of the form submission or confirmation logic. This could lead to stale data if the form state changes before submission. Consider moving this logic inside the onSubmit or onConfirm functions.
| forceUpdateValue | ||
| onChange={_.noop} | ||
| error={_.get(errors, 'newHandle.message')} | ||
| inputControl={register('newHandle')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 maintainability]
The inputControl prop is not a standard prop for InputText. If this is a custom implementation, ensure it is correctly handled within the InputText component. Otherwise, consider removing or renaming it to avoid confusion.
| .then(result => { | ||
| setIsLoading(false) | ||
| toast.success('Handle updated successfully') | ||
| props.userInfo.handle = result?.handle ?? nextHandle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[design]
Directly mutating props.userInfo.handle could lead to unexpected side effects, especially if userInfo is used elsewhere. Consider using a state update function or a context provider to manage this state change.
| {showDialogEditUserHandle && ( | ||
| <DialogEditUserHandle | ||
| open | ||
| setOpen={function setOpen() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡 maintainability]
Consider using a consistent naming convention for the setOpen function across all dialog components. Currently, the setOpen function is defined inline with a different style compared to other similar functions in the file. This could improve maintainability by ensuring consistency.
| * Model for edit user handle form | ||
| */ | ||
| export interface FormEditUserHandle { | ||
| newHandle: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Consider adding validation logic to ensure that newHandle meets expected constraints (e.g., length, allowed characters) to prevent potential issues with invalid user handles.
| newHandle: string, | ||
| ): Promise<MemberInfo> => { | ||
| const payload = { | ||
| newHandle: newHandle.trim(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Consider validating newHandle before trimming and sending it in the payload. This ensures that the handle meets any necessary criteria (e.g., length, character restrictions) before making the API call.
| } | ||
|
|
||
| return xhrPatchAsync<typeof payload, MemberInfo>( | ||
| `${EnvironmentConfig.API.V6}/members/${encodeURIComponent(handle)}/change_handle`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[❗❗ security]
Ensure that handle is validated before encoding and using it in the URL. This prevents potential issues with invalid or malicious input.
| */ | ||
| export const formEditUserHandleSchema: Yup.ObjectSchema<FormEditUserHandle> | ||
| = Yup.object({ | ||
| newHandle: Yup.string() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[correctness]
Consider adding a .min(3, 'Handle must be at least 3 characters long.') constraint to ensure the handle has a minimum length. This can help prevent invalid or too short handles from being accepted.
topcoder.atlassian.net/browse/PS-513